feat(init): add audience prompt for admin-provided vs DIY OAuth setup by rianjs · Pull Request #124 · open-cli-collective/google-readonly

rianjs · 2026-05-13T22:14:41Z

Summary

Adds a top-level SelectAudience prompt to gro init that routes users between two prefatory messages: admin-provisioned (paste/point to credentials.json shared via 1Password etc.) and DIY (the existing Google Cloud Console setup steps, now augmented with a pointer to WORKSPACE_ADMINS.md for admins who'd rather provision once for their whole org).
Both paths fall into the same SelectCredSource → save logic. --credentials-file still bypasses the wizard entirely.
SelectAudience is asked exactly once per wizard invocation — sticky across credential-validation retries.

Why now

PR #122 merged WORKSPACE_ADMINS.md. Today, every user who runs gro init sees Google Cloud Console setup steps even if their admin already handed them a credentials.json. This PR aligns the wizard with the new doc so neither audience reads past content they don't need.

Tests

Three new tests in init_test.go:

TestEnsureCredentialsAudienceAdminSkipsDIYSteps — admin audience does not print the 7-line DIY block but does print the admin one-liner.
TestEnsureCredentialsAudienceDIYShowsDIYStepsAndAdminPointer — DIY audience prints the existing block plus a fully-qualified link to WORKSPACE_ADMINS.md on GitHub.
TestEnsureCredentialsAudienceIsAskedOncePerWizard — sticky behavior: clipboard returns invalid JSON on call 1 and valid JSON on call 2; `audience` is called exactly once and `select` exactly twice across the retry.

Two existing bypass tests extended to assert `audience` is NOT called:

`TestEnsureCredentialsFlagFile` (--credentials-file bypass)
`TestEnsureCredentialsShortCircuitsWhenAlreadyPresent` (existing creds.json bypass)

Test plan

`make check` passes (was green locally; 49 tests pass in initcmd)
Manual: `rm -f ~/.config/google-readonly/credentials.json && gro config clear` then `gro init` — admin path shows no DIY block, DIY path shows block + admin doc URL.

Closes #123

A new top-level wizard question routes users between two prefatory messages: admin-provisioned users get a one-liner pointing at the credentials.json their admin shared; DIY users get the existing Google Cloud Console setup steps plus a pointer to WORKSPACE_ADMINS.md for admins who want to provision once for their whole org. Both paths flow into the same SelectCredSource → save logic. The --credentials-file flag still bypasses the wizard entirely. SelectAudience is asked exactly once per wizard invocation; sticky across credential-validation retries. Closes #123

rianjs · 2026-05-13T22:16:02Z

Minor

internal/cmd/initcmd/init.go:60: cmd.Long still says “see WORKSPACE_ADMINS.md in the repository.” Installed CLI users do not have that file locally. Use the same fully-qualified GitHub URL as the DIY wizard text.
internal/cmd/initcmd/init_test.go:452: the DIY pointer test only asserts WORKSPACE_ADMINS.md, so it would pass if the wizard regressed back to a bare filename. Assert the full URL to lock the product requirement.

Nit

internal/cmd/initcmd/init.go:434: consider a switch audience with a default error, matching SelectCredSource. The current else silently treats any unexpected value as DIY.

Code Quality Lens: the core architecture is right: audience only changes prefatory text, both branches share credential source/save logic, retries keep audience sticky, and both bypasses are preserved. Remaining issues are small user-facing/help-text consistency and test precision gaps.

- Use fully-qualified WORKSPACE_ADMINS.md URL in cmd.Long (matches the wizard text; reachable by installed-CLI users) - Tighten DIY-pointer test assertion to the full URL so a regression to bare filename would fail - Switch audience branch to switch/default-error pattern, matching the SelectCredSource pattern; rejects unexpected prompter values

rianjs · 2026-05-13T22:17:34Z

Findings

None.

The updated PR now matches the intended architecture: SelectAudience only controls prefatory text, both audiences flow into the same SelectCredSource and save path, retries keep the audience decision sticky, and both --credentials-file plus existing-credentials bypasses remain prompt-free.

Code Quality Lens: no scope creep, no unrelated edits, no hidden local-doc assumption, and the important behavior is covered by focused tests.

rianjs · 2026-05-13T22:18:09Z

TL;DR: Coverage is adequate. Optional nit below; not blocking.

Walked through the diff against init_test.go:

Admin branch — covered (TestEnsureCredentialsAudienceAdminSkipsDIYSteps, asserts DIY steps absent + admin one-liner present).
DIY branch — covered (TestEnsureCredentialsAudienceDIYShowsDIYStepsAndAdminPointer, asserts DIY steps + fully-qualified WORKSPACE_ADMINS.md URL).
Sticky-across-retries — covered (TestEnsureCredentialsAudienceIsAskedOncePerWizard with a 2-attempt clipboard retry, asserts audience==1 and select==2). This is the test I'd most want to see and it's here.
--credentials-file bypass — covered with a negative assertion that SelectAudience is not in stub.calls.
Existing-creds short-circuit — covered with the same negative assertion.

Gap: the default: arm of the audience switch (unknown audience: %s) is not exercised. The producer (huhPrompter.SelectAudience) only ever returns "admin" or "diy", so this is purely defensive against future refactors. Pragmatic call — fine to leave as-is. If you want belt-and-suspenders, a 5-line test setting stubPrompter{audience: "bogus"} and asserting the error would do it, but I wouldn't block on it.

Test patterns match the rest of the file (parallel, baseDeps, stubPrompter, output-buffer assertions). Ship it.

monit-reviewer · 2026-05-13T22:20:55Z

+		t.Errorf("diy audience should show DIY steps; output:\n%s", got)
+	}
+	if !strings.Contains(got, "https://github.com/open-cli-collective/google-readonly/blob/main/WORKSPACE_ADMINS.md") {
+		t.Errorf("diy audience should show the fully-qualified Workspace admin doc URL (so Homebrew/Choco users can reach it); output:\n%s", got)


🟡 Medium (harness-engineering:harness-self-documenting-code-reviewer): TestEnsureCredentialsAudienceIsAskedOncePerWizard accesses reads[idx] without bounds checking. If ClipboardReadAll is called more than twice (e.g., after a refactor raising the retry cap), the test panics with an index-out-of-range instead of failing cleanly. Use a bounds-checked helper or t.Fatal inside the closure.

Reply to this thread when addressed.

Fixed in 8a40d4d — the ClipboardReadAll closure now checks idx >= len(reads) and calls t.Fatalf if the wizard makes more than 2 reads, so a future retry-cap bump fails cleanly instead of panicking with index-out-of-range.

monit-reviewer · 2026-05-13T22:20:55Z

 	calls []string
 }

+func (s *stubPrompter) SelectAudience() (string, error) {


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): stubPrompter.SelectAudience() silently returns "diy" when the audience field is the zero value (empty string). Any existing or future test that constructs a &stubPrompter{} without setting audience will quietly exercise the diy path rather than failing. A sentinel value or required-non-empty check would surface misconfigured stubs earlier.

Reply to this thread when addressed.

Declining — this was a deliberate design choice flagged in the architect review for this PR. The zero-value default to 'diy' was specifically requested to avoid touching ~20 existing test setups that would otherwise need to set audience: "diy" to keep compiling. Existing tests that exercise the wizard exercised the DIY path before this PR; defaulting their zero-value to 'diy' preserves that semantic exactly. New tests that care about audience set it explicitly. The two bypass tests (TestEnsureCredentialsFlagFile, TestEnsureCredentialsShortCircuitsWhenAlreadyPresent) now assert audience is NOT in calls, which catches the misuse case you're worried about: if a wizard test forgot to think about audience and the audience prompt fires unexpectedly, those tests would surface it.

monit-reviewer

Automated PR Review

Reviewed commit: 6821029

Summary

Reviewer	Findings
harness-engineering:harness-architecture-reviewer	1
harness-engineering:harness-self-documenting-code-reviewer	2

harness-engineering:harness-architecture-reviewer (1 findings)

💡 Suggestion - internal/cmd/initcmd/init.go:477

The WORKSPACE_ADMINS.md URL is hardcoded in two places: the cobra command's Long description and the runtime Println in the diy case. A single package-level constant would eliminate drift if the repo is renamed or the doc is moved.

harness-engineering:harness-self-documenting-code-reviewer (2 findings)

⚠️ Should Fix - internal/cmd/initcmd/init_test.go:458

TestEnsureCredentialsAudienceIsAskedOncePerWizard accesses reads[idx] without bounds checking. If ClipboardReadAll is called more than twice (e.g., after a refactor raising the retry cap), the test panics with an index-out-of-range instead of failing cleanly. Use a bounds-checked helper or t.Fatal inside the closure.

💡 Suggestion - internal/cmd/initcmd/init_test.go:155

stubPrompter.SelectAudience() silently returns "diy" when the audience field is the zero value (empty string). Any existing or future test that constructs a &stubPrompter{} without setting audience will quietly exercise the diy path rather than failing. A sentinel value or required-non-empty check would surface misconfigured stubs earlier.

3 PR discussion threads considered.

Completed in 2m 30s | $0.71 | sonnet | daemon 0.2.116 | Glorfindel

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	2m 30s wall · 7m 14s compute (Reviewers: 1m 48s · Synthesis: 40s)
Cost	$0.71
Tokens	186.7k in / 28.9k out
Turns	10

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	30.1k	2.0k	13.6k	16.5k (1h)	$0.10
harness-engineering:harness-architecture-reviewer	sonnet	35.0k	4.5k	19.8k	15.2k (1h)	$0.14
harness-engineering:harness-enforcement-reviewer	sonnet	36.0k	5.3k	19.8k	16.2k (1h)	$0.15
harness-engineering:harness-knowledge-reviewer	sonnet	35.2k	4.5k	19.8k	15.4k (1h)	$0.14
harness-engineering:harness-self-documenting-code-reviewer	sonnet	34.2k	3.5k	19.8k	14.4k (1h)	$0.12
security:security-code-auditor	haiku	16.2k	9.1k	0	16.2k (1h)	$0.07

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

Note: Posted 2 inline comments. 1 finding remained in the summary because GitHub could not attach them to the current diff.

…e test - Extract workspaceAdminsURL to a package-level const, referenced from both cmd.Long (via fmt.Sprintf) and the runtime wizard. Eliminates drift if the repo moves or the doc is renamed. - Bounds-check ClipboardReadAll closure in TestEnsureCredentialsAudienceIsAskedOncePerWizard so a future retry-cap bump fails cleanly with t.Fatal instead of an index-out-of-range panic. Addresses two pr-review-daemon findings on PR #124.

rianjs · 2026-05-13T22:22:45Z

Re. the URL-constant suggestion from harness-architecture-reviewer: addressed in 8a40d4d. Extracted workspaceAdminsURL to a package-level const referenced from both cmd.Long (via fmt.Sprintf) and the runtime wizard.

The bounds-check finding is fixed in the same commit. The audience zero-value default suggestion is being declined with rationale on the inline thread.

monit-reviewer reviewed May 13, 2026

View reviewed changes

monit-reviewer requested changes May 13, 2026

View reviewed changes

rianjs merged commit e906613 into main May 13, 2026
2 checks passed

rianjs deleted the feat/123-init-audience-prompt branch May 13, 2026 22:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(init): add audience prompt for admin-provided vs DIY OAuth setup - #124

feat(init): add audience prompt for admin-provided vs DIY OAuth setup#124
rianjs merged 3 commits into
mainfrom
feat/123-init-audience-prompt

rianjs commented May 13, 2026

Uh oh!

rianjs commented May 13, 2026

Uh oh!

rianjs commented May 13, 2026

Uh oh!

rianjs commented May 13, 2026

Uh oh!

monit-reviewer May 13, 2026

Uh oh!

rianjs May 13, 2026

Uh oh!

monit-reviewer May 13, 2026

Uh oh!

rianjs May 13, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

rianjs commented May 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

rianjs commented May 13, 2026

Summary

Why now

Tests

Test plan

Uh oh!

rianjs commented May 13, 2026

Uh oh!

rianjs commented May 13, 2026

Uh oh!

rianjs commented May 13, 2026

Uh oh!

monit-reviewer May 13, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs May 13, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer May 13, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs May 13, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

rianjs commented May 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants